Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fixed getting error in console while selecting all downloadable links #24633 #24634

Merged
merged 1 commit into from
Sep 26, 2019
Merged

fixed getting error in console while selecting all downloadable links #24633 #24634

merged 1 commit into from
Sep 26, 2019

Conversation

rani-priya
Copy link
Contributor

… #24633

Description (*)

Fixed Issues (if relevant)

  1. Getting error in console while selecting all downloadable links  #24633: Getting error in console while selecting all downloadable links

Manual testing scenarios (*)

  1. Create a downloadable product with more than one downloadable links
  2. Open that product at frontend
  3. click on select all link
  4. open developer tool and check console
    downloadable-issue

Questions or comments

Contribution checklist (*)

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All new or changed code is covered with unit/integration tests (if applicable)
  • All automated tests passed successfully (all builds are green)

@m2-assistant
Copy link

m2-assistant bot commented Sep 18, 2019

Hi @rani-webkul. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

  • @magento give me test instance - deploy test instance based on PR changes
  • @magento give me 2.3-develop instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Guide documentation.

@ghost ghost assigned ihor-sviziev Sep 18, 2019
@ihor-sviziev ihor-sviziev removed the request for review from akaplya September 18, 2019 06:04
ihor-sviziev
ihor-sviziev requested changes Sep 18, 2019
View reviewed changes
Copy link
Contributor

@ihor-sviziev ihor-sviziev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @rani-webkul,
Your changes looks good to me.

  1. Could you squash your changes to single commit?
  2. Would be great to cover your changes with some tests. Will you be able to do that?

@ihor-sviziev ihor-sviziev added the Auto-Tests: Not Covered Changes in Pull Request requires coverage by auto-tests label Sep 18, 2019
@rani-priya rani-priya removed the request for review from DrewML September 18, 2019 07:18
@rani-priya
Copy link
Contributor Author

@ihor-sviziev ,

Could you squash your changes to single commit?
-> done
Would be great to cover your changes with some tests. Will you be able to do that?
-> will do that

@ghost
Copy link

ghost commented Sep 19, 2019

@rani-webkul unfortunately, only members of the maintainers team are allowed to remove progress related labels to the pull request

@ihor-sviziev
Copy link
Contributor

Hi @rani-webkul,
Waiting for tests coverage, then will approve your PR. If you have any questions - let me know.

@rani-priya
Copy link
Contributor Author

@ihor-sviziev could you please help me for tests coverage?

@ihor-sviziev
Copy link
Contributor

ihor-sviziev commented Sep 19, 2019 •
edited
Loading

Hi @rani-webkul,

There we could add MFTF test for this feature. Docs are here: https://devdocs.magento.com/mftf/docs/introduction.html

The most close test case I think is following: https://github.com/magento/magento2/blob/e93feb02398f158f79fefb14de95cf93e3ec0786/app/code/Magento/Downloadable/Test/Mftf/Test/VerifyDisableDownloadableProductSamplesAreNotAccessibleTest.xml

BTW it's not hard requirement, we could approve it without tests, but it's highly recommended to cover all changes in order not to add regression in future.

@rani-priya
Copy link
Contributor Author

Hi @ihor-sviziev , Thanks for helping.. I'm now writing the test coverage for this PR.
But facing one issue that how can I verify or see, there is no error in console?

@ihor-sviziev
Copy link
Contributor

ihor-sviziev commented Sep 19, 2019 •
edited
Loading

@rani-webkul, as I understood that JS error caused broken functionality "select all"/"unselect all", idea is just add checking that this functionality works fine, not check that we don't have JS errors.

Also you could look at https://github.com/magento/magento2/pull/24580/files#diff-5ea8a7b56c95d845707926c666af76c4 . It's still not merged, but that's your PR, it might help you

ihor-sviziev
ihor-sviziev requested changes Sep 19, 2019
View reviewed changes
Copy link
Contributor

@ihor-sviziev ihor-sviziev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @rani-webkul,
Please check my comments

...de/Magento/Downloadable/Test/Mftf/Test/SelectAllDownloadableLinksDownloadableProductTest.xml Outdated Show resolved Hide resolved
...de/Magento/Downloadable/Test/Mftf/Test/SelectAllDownloadableLinksDownloadableProductTest.xml Outdated Show resolved Hide resolved
@ihor-sviziev ihor-sviziev added Auto-Tests: Covered All changes in Pull Request is covered by auto-tests Award: MFTF test coverage Award: test coverage and removed Auto-Tests: Not Covered Changes in Pull Request requires coverage by auto-tests labels Sep 19, 2019
ihor-sviziev
ihor-sviziev requested changes Sep 19, 2019
View reviewed changes
Copy link
Contributor

@ihor-sviziev ihor-sviziev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @rani-webkul,
Unfortunately functional tests failed. Please review & fix them.

@magento-engcom-team
Copy link
Contributor

Hi @ihor-sviziev, thank you for the review.
ENGCOM-5865 has been created to process this Pull Request

@engcom-Delta engcom-Delta self-assigned this Sep 20, 2019
@engcom-Delta
Copy link
Contributor

✔️ QA passed

@engcom-Foxtrot engcom-Foxtrot self-assigned this Sep 20, 2019
@magento-engcom-team magento-engcom-team mentioned this pull request Sep 26, 2019
Closed
magento-engcom-team pushed a commit that referenced this pull request Sep 26, 2019
@sidolov
ENGCOM-5865: fixed getting error in console while selecting all downl…
43c5dff 
…oadable links #24633 #24634
@magento-engcom-team magento-engcom-team merged commit 66b7785 into magento:2.3-develop Sep 26, 2019
@m2-assistant
Copy link

m2-assistant bot commented Sep 26, 2019

Hi @rani-webkul, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.

@magento-engcom-team magento-engcom-team added this to the Release: 2.3.4 milestone Sep 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ihor-sviziev ihor-sviziev ihor-sviziev approved these changes

Assignees

@ihor-sviziev ihor-sviziev

@engcom-Delta engcom-Delta

@engcom-Foxtrot engcom-Foxtrot

Labels
Area: Frontend Auto-Tests: Covered All changes in Pull Request is covered by auto-tests Award: bug fix Award: MFTF test coverage Component: Downloadable Component: Wishlist Progress: accept Release Line: 2.3
Projects
None yet
Milestone
Release: 2.3.4
Development

Successfully merging this pull request may close these issues.
5 participants
@rani-priya @ihor-sviziev @magento-engcom-team @engcom-Delta @engcom-Foxtrot